-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix type generator test failures #44041
Merged
davidwrighton
merged 11 commits into
dotnet:master
from
davidwrighton:fix_type_generator_tests
Nov 1, 2020
Merged
Fix type generator test failures #44041
davidwrighton
merged 11 commits into
dotnet:master
from
davidwrighton:fix_type_generator_tests
Nov 1, 2020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ived type but is actually defined on a base type - Make MethodWithToken which is reliably IL token dependent always be generated in the CorInfoImpl class, and pass the appropriate context to the MethodWithToken constructor so that it can find the appropriate OwningType.
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Oct 30, 2020
davidwrighton
added
area-crossgen2-coreclr
and removed
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
labels
Oct 30, 2020
davidwrighton
changed the title
Fix crossgen2 issues where method reffered to derived type but method was actually defined on base type
Fix crossgen2 issues where method referred to derived type but method was actually defined on base type
Oct 30, 2020
…ct OwningType - The OwningType needs to be associated with the token of the signature, not the final resolved method
davidwrighton
changed the title
Fix crossgen2 issues where method referred to derived type but method was actually defined on base type
Fix type generator test failures
Oct 30, 2020
trylek
approved these changes
Oct 31, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me, I'm amazed by the cleanness and simplicity of your fixes, thank you!
...ls/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/DelegateCtorSignature.cs
Outdated
Show resolved
Hide resolved
… OwningType computation
This was referenced Nov 2, 2020
ghost
locked as resolved and limited conversation to collaborators
Dec 6, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix issue where method token referred to derived type but method was actually defined on base type
Fix issue where constrained dispatch on a method of a valuetype would not put in the correct owner type
In general these issues where both caused by confusion around exactly the correct owning type, and it turned out that computation could not be computed within the signature emitter code, but instead needed to be computed in the JIT at point of use. Fortunately, we had a structure
MethodWithToken
that is used in these (and only these situations). Finally, I also made a pass through the emitter and related logic to remove various band-aids that had built up over the last few years to make all the tests and applications pass. I believe that the new logic should be correct in the general case.Bonus tweak... Use parallelism when compiling the framework composite images with crossgen2, and fix bug in composite image generation where mangled symbol names might conflict.
Fixes #43466 and fixes #43467